tools - #1751

Backported from cd72548 in the #1751-#1774 stack for the hotfix release. Test hunks deferred.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Backported from 2f7a66d in the #1751-#1774 stack for the hotfix release. Test hunks deferred.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Two packets of one new flow can reach a NAT stage at the same time.
Packets of a 5-tuple usually land on one core, but nothing guarantees
that, and each packet builds a pair of its own before inserting it.

With a plain insert, whoever gets there second displaces the other's
forward flow -- and only that half. The two reverse keys carry the
allocations that made them, no two allocations agree, so the reverses
never collide and the loser's is never displaced along with its partner.
It stays in the table, live, mapping a translation whose allocation goes
back to the pool as soon as the displaced forward half is collected.
Return traffic for that public pair, once it has been handed out again,
is then translated for whoever held it before.

Two changes, either of which leaves a hole on its own.

insert_if_absent stands aside when a live flow already holds the key,
and reports that flow so the caller can go on with it. Arbitrating on
one key is enough, because racing packets of a single flow share their
forward key by construction: only whoever wins it inserts a reverse. A
flow that is present but no longer live is displaced as before, since it
is a corpse its timer has not swept yet and standing aside for one would
drop a packet that could have replaced it.

Displacing a flow now also invalidates the other half of its pair,
wherever it happens. The race is not the only way to reach the orphan:
the flow timer expires the two halves separately, so an expired forward
half could be replaced by an ordinary insert while its partner was still
live. That path needs no concurrency at all.

Three tests, each of which fails against the code without its guard: a
live flow keeps its key, a dead one does not, and displacing a flow
takes its partner with it.

Rebased onto a `related_pair` that is fallible and requires exactly one
half of the pair to carry `INITIATOR`; both are invariants main gained
after this was written. The test now marks the forward half and unwraps,
matching the sibling test in `concurrent_fuzz.rs`.

Backported from 0327e5e in the #1751-#1774 stack for the hotfix release. Test hunks deferred.

Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

A masqueraded connection is two flow entries, forward and reverse.
`refresh_masquerade_state` refreshed only the one a packet happened to hit; the
partner was refreshed exactly once, on the transition into `Established`.

So a connection whose traffic runs mostly one way lets the other half expire
while it is still in use -- and that is the common case rather than a corner. A
download is almost all reverse packets. So is a DNS response, or any session that
mostly receives.

It is worse than a dropped connection. `MasqueradeState` carries the `Allocation`
in the **forward** entry alone, so the forward half expiring releases the address
and port while the reverse half goes on translating to them. The allocator hands
that tuple to another tenant, whose replies arrive at the first tenant's
still-live reverse entry. Two tenants sharing one public tuple is a tenant
isolation failure, not a routing one, and nothing on the packet path shows it:
every reply to the first tenant keeps being delivered correctly the whole time.
Only a *new* flow taking the tuple reveals it.

The fix is to refresh the partner on every refresh rather than on one transition.
`reset_expiry_unchecked` refuses to move a deadline earlier, so extending the
partner can only lengthen its life. This is what conntrack has always done: a
packet in either direction is evidence the whole connection is alive.

Reaching this on the wall clock needs five seconds of real time per attempt plus
the right allocation pattern -- and `randomize(true)` in production makes the
reissue unlikely rather than impossible, so it would surface in the field as a
rare, unreproducible cross-tenant delivery.

The `#[ignore]`d reproduction is replaced by two passing properties:

  * `both_halves_of_a_pair_outlive_one_sided_traffic` -- the mechanism, asserted
    on the live flow count, because the packet path cannot see it; and
  * `a_live_flows_tuple_is_never_reissued` -- the outcome an operator would feel,
    kept separate because it would also catch a different allocator bug that
    released a tuple for some other reason.

Backported from 3bf1831 in the #1751-#1774 stack for the hotfix release. Test hunks deferred.

Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`fix(masquerade): Keep both halves of a flow pair alive` made this argument for
masquerade and left the identical code in port forwarding: refresh only the half
a packet happened to hit, except on the transition into Established.

A pair is one connection, and a packet in either direction is evidence the whole
thing is alive. Refreshing one half lets the other expire under a live
connection whenever traffic runs mostly one way -- and for a published service
that is the ordinary case, not a corner. A client uploading refreshes the
forward half on every packet while the reverse half, which carries the
translation its replies need, times out beneath it.

Milder than masquerade's, deliberately said so in the comment: port forwarding
maps from the rule rather than from an allocation, so nothing is released to the
pool and no tenant sees another tenant's traffic. What is lost is the
connection.

`reset_expiry_unchecked` refuses to move a deadline earlier, so extending the
partner can only lengthen its life.

Backported from 817b0fb in the #1751-#1774 stack for the hotfix release. Test hunks deferred.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>

`reset_expiry_unchecked` loaded the stored deadline, compared, and stored -- three
steps. Two threads could therefore both pass the guard against the same stale
value and the shorter write land last, which is the deadline moving *backwards*:
the one thing that comparison exists to prevent.

`fix(masquerade): Keep both halves of a flow pair alive`, four commits back, is
what makes this ordinary rather than exotic. Before it a packet refreshed the half
it hit; now both directions refresh both halves on every packet, and `extend_by`
differs by status, so concurrent writes of *different* deadlines to one location
are the normal case for any bidirectional flow.

Losing the longer write on the forward half is the serious outcome, because that
half owns the `Allocation`: it expires under a live connection and the allocator
hands its public tuple to another tenant -- exactly the failure refreshing both
halves was meant to stop, reached by a narrower path.

The guard stays strictly greater, so resetting to the deadline already held is
still accepted; `the_unchecked_refreshes_move_the_deadline_exactly` pins that
boundary deliberately and rejects the `>=` spelling in under a millisecond.

No regression test, and the comment says why at length: tsan cannot see an
atomicity violation between two atomic operations, a 200,000-round stress test
scores zero, and the model checker that would settle it needs `AtomicInstant`
routed through `concurrency` first.

Backported from 9a331a8 in the #1751-#1774 stack for the hotfix release. Clock facade elided.

Signed-off-by: Daniel Noland <daniel@githedgehog.com>

`NatPool::for_range` seeds the bitmap with every offset in the region, capped at
`u32::MAX`. Any public range of /96 or shorter therefore saturates at 2^32
entries, and a v6 /64 -- the size an operator actually writes -- is exactly that.

`ips_in_bitmap` walked those one bit at a time, doing a `BTreeMap` lookup per
offset to turn it into an address and then coalescing the results back into
ranges: four billion lookups to print, for a pool with nothing allocated, one
line. `roaring`'s `Iter::next_range` yields a whole run per step, so the cost is
one iteration per run and runs are bounded by what has been allocated.

The lock mattered more than the minutes. `Display` held the pool's read guard for
the entire print while `allocate_ip` and `deallocate_ip` want it for writing, so
rendering a v6 pool stalled every new masqueraded flow in it. The guard now covers
a snapshot -- the ranges, and an upgrade of each live address -- and formatting
happens after it is released. That upgrade is still what makes releasing safe:
`AllocatedIp::drop` takes the same lock for writing, and holding a strong
reference means none of the addresses examined here can be the last.

The hand-rolled coalescing goes with it, `unreachable!()` arm included. A run of
offsets is a run of addresses only while the mapping is a single linear
translation, which is all `for_range` builds; that is now asserted rather than
assumed, because the type is shaped to hold more.

The regression test uses a stopwatch for an oracle without asserting on one:
before this, printing a /64 pool does not return, and a test that does not return
is a failure the runner reports by itself.

Backported from 817593a in the #1751-#1774 stack for the hotfix release. Test hunks deferred.

Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Standing aside for a live incumbent asks the table to hold nothing, so a full
table has no reason to refuse it -- and refusing turned the one answer a racing
caller can act on into an error it can only drop a packet over, precisely where
races are most likely.

`admit` reads the table's length, and `DashMap::len` read-locks every shard,
including the one an entry guard holds. Hence the length is read up front and
consulted inside the vacant arm alone, rather than the admission check simply
moving down.

Backported from 1221da3 in the #1751-#1774 stack for the hotfix release. Test hunks deferred.

Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`insert_if_absent` was added with no caller, so the race it describes was still
open: two packets of one new flow, handled at the same moment on two cores, each
allocated a public tuple and each installed a pair. The second displaced the
first's forward half, and the first's reverse half -- keyed on an allocation
nothing else can collide with -- stayed live, translating for a tuple that goes
back to the pool with the half that was displaced and is then handed to somebody
else.

The loser keeps nothing. Its reverse half is never inserted, and dropping the
pair it built releases the allocation through `AllocatedPort`, so there is no
hand-written release path to keep in step. It masquerades with the winner's
flow, which is the answer the packet after it would have got anyway.

Re-checking and teardown stay with whoever installed the flow. A flow won by
another packet was built from an allocator handle of its own, and judging it by
this packet's could invalidate a sound flow over an allocator it never used.

Backported from 25967df in the #1751-#1774 stack for the hotfix release. Test hunks deferred.

Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

No concurrency is needed to reach this one. `process_packet` takes the slow path
on the stamp alone -- `get_packet_port_fw_state` reads what `FlowLookup`
attached and never falls back to the table, unlike masquerade -- and a burst is
stamped before any of it is forwarded. Two packets of one new flow in a single
burst therefore both build a pair, and the second tears the first's down.

Sequentially that ends consistent, which is why it went unseen: the second pair
carries the same translation, so a connection that has been rebuilt underneath
its own first packets forwards exactly like one that was left alone. Two cores
end it worse. Displacing a flow invalidates its partner, so a loser's reverse
insert landing after the winner's strikes down the winner's forward half and
then stays live under it -- a pair that answers replies it can no longer
forward.

Backported from 8d63cde in the #1751-#1774 stack for the hotfix release. Test hunks deferred.

Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`insert_if_absent` decides by asking whether the incumbent `is_active()`, and
`FlowInfo::new` starts a flow at `Detached`. Publishing the status after the flow
is reachable leaves a window in which it is in the map and still reads as a
corpse: a caller landing in that window displaces a live flow and reports
`Installed` too.

Two winners for one key is what this method exists to prevent. Both then install
a reverse flow, and the loser's reverse outlives the allocation it maps -- which
is the failure the masquerade and port-forwarding arbitration in the two commits
below this one were written to close.

`insert_common` does not need it. Its window is harmless because the only reader
of the status is `drain_stale`, which also requires the deadline to have passed,
and a fresh flow expires in the future. This method is the reader that window
became visible to.

Set before taking the entry guard rather than under it, which is the obvious
answer and a trap: `update_status` writes a `concurrency` atomic, so it is a
scheduling point under shuttle, while `dashmap` is a plain dependency whose shard
locks shuttle cannot see. A preemption inside a lock the model checker does not
know about parks every green thread on the one OS thread -- the suite deadlocks
at 0% CPU rather than reporting anything. Nothing observes `val` before it is
inserted, so publishing early costs nothing; it only has to be undone on the two
paths that do not insert, and those are outside the guard.

That is also why there is no regression test. A shuttle property over two threads
inserting one key hangs for the same reason; instrumenting `dashmap` is the
prerequisite, and the doc comment now says so.

Backported from 70ce47d in the #1751-#1774 stack for the hotfix release. Test hunks deferred.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Daniel Noland <daniel@githedgehog.com>

`FlowFilter` collects the whole rx burst before anything downstream of it runs, so every
packet of a burst carries a flow stamp taken before any of them was masqueraded. Reading
only the stamp meant sixteen UDP packets of one flow in one burst took sixteen ports out
of the pool and reached the far side as sixteen sources, and a TCP SYN with its first data
segment in one burst had the data dropped as "TCP without SYN".

Found by the pipeline harness, by probing the case its burst property excludes.

Backported from 538ee4b in the #1751-#1774 stack for the hotfix release. Test hunks deferred.

Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Consulting the flow table made a flow the burst is itself establishing visible to
the rest of that burst, but only where the stamp was absent. A stamp that is
present and unusable -- expired, cancelled, or carrying no masquerade state --
returned `None` and went straight to allocation.

A burst is stamped before any of it is masqueraded, so that is not a rare case:
if the entry dies between the stamp and the stage, every packet of the burst
holds the same dead `Arc`. The SYN replaces the entry, and the data segment
behind it never sees the replacement -- it is refused as "TCP without SYN", which
is the failure the table lookup was added to prevent. Insert-time arbitration
does not cover it, because that refusal happens before any insert.

Backported from 18109d3 in the #1751-#1774 stack for the hotfix release. Test hunks deferred.

Signed-off-by: Daniel Noland <daniel@githedgehog.com>

Masquerade combined with static NAT allocated a fresh public tuple for
every packet of a burst: eight packets of one flow left under eight public
tuples and put eight reverse entries in the table.

The pipeline runs `static_nat` before `masquerade`, so by the time
masquerade sees a packet its destination has already been rewritten and the
key it carries is no longer the key `FlowLookup` used. `create_flow_pair`
files the forward flow under the *initial* key for exactly that reason --
`FlowLookup` runs first and would otherwise never find the flow again --
while the intra-burst fallback in `get_masquerade_state` looked it up under
the key the packet carries now.

Equal whenever nothing translated the destination, which is why one lookup
sufficed. The fallback is tried second rather than first so this only adds a
lookup where the old code found nothing: a reply is keyed on
`new_reverse_session`'s derivation of the current key and must keep
matching first.

The combination was unreachable from the two-vpc fixture, whose far side is
always one plain prefix; `overlay_between` lets a test name both sides.

Backported from 8f1b834 in the #1751-#1774 stack for the hotfix release. Test hunks deferred.

Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>